Use std::optional to clean up some code and prevent use-after-free bugs #8484
Conversation
|
clang-tidy is flagging this as an unchecked use of a std::optional: ... that's checked, right? We also have it complaining about this: because it doesn't see the assert as checking. Perhaps I should just disable that clang-tidy check. |
| * the expression if it is, or std::nullopt if not. Also returns std::nullopt | ||
| * for non-integer types. */ | ||
| // @{ | ||
| std::optional<int> is_const_power_of_two_integer(const Expr &e); |
There was a problem hiding this comment.
style nit: is_foo implies the function returns bool, which these no longer do. probably as_const_foo is now a better name for these.
There was a problem hiding this comment.
The issue is that it doesn't return a const power of two. It returns the number of bits in it, if it is one. So "as" is actively misleading. I couldn't immediately think of a better name. If you treat the return value as a bool, "is" still makes sense. Open to other suggestions. exact_integer_log_2?
There was a problem hiding this comment.
Ah, ok then... name is fine I guess
src/CodeGen_Internal.cpp
Outdated
| if (!value) { | ||
| return false; | ||
| return {}; |
There was a problem hiding this comment.
style q: if this preferable to returning std::nullopt? I would have thought the latter to be better, but whatever...
There was a problem hiding this comment.
It felt natural to me because it looks like the empty set, and "nullopt" didn't seem like it communicated anything useful (there's no null pointer involved, so why "null"). But I don't feel strongly and can change it if you think it would be an improvement.
There was a problem hiding this comment.
Returning nullopt is a good habbit, as it's ambiguous for struct types that take an empty constructor. What is std::optional<std::string> test = {}? I honestly don't know... https://godbolt.org/z/G683WKTxa Godbolt shows it will take the nullopt, instead of the no-args-constructed object.
There was a problem hiding this comment.
I don't agree it's ambiguous, because you'd have to say {{}} to get an optional empty-constructor struct. {} is always just default construction of the actual return type.
But that said, that's the third time it's given someone pause when reading it (Alex also mentioned it offline), so I'm definitely changing it to nullopt at this point.
|
Personal taste: I dislike the use of |
It depends on the context IMHO. When it avoid repeating a type it's a clear win. When it omits the type entirely, less so -- IMHO no one should have to use an IDE to be able to easily read the code. |
|
In this case I did it for two reasons:
|
|
Ready to land? |
A lot of this code was pre C++-17, and could be improved using std::optional. This would have avoided #8482 and arguably the bug underlying #8469